-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse invalid property under target #8190
Conversation
The current error after change is error MSB4067: The element beneath element is unrecognized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions:
Should we add a change wave?
Should we have a custom message so we can suggest that they probably intended to wrap it in a PropertyGroup?
format equals Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>
What's the change wave? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a change wave?
Should we have a custom message so we can suggest that they probably intended to wrap it in a PropertyGroup?What's the change wave? How to add a custom message? From build output message?
I like @Forgind's suggestions.
For change waves, please read https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves-Dev.md and ask any follow-up questions--I'm sure that doc can be improved.
For the new error message, I commented in the PR.
I would also like to see a test added, probably like this one and in this file:
msbuild/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs
Lines 160 to 177 in d797c48
/// <summary> | |
/// Read a target with a missing name | |
/// </summary> | |
[Fact] | |
public void ReadInvalidMissingName() | |
{ | |
Assert.Throws<InvalidProjectFileException>(() => | |
{ | |
string content = @" | |
<Project> | |
<Target/> | |
</Project> | |
"; | |
ProjectRootElement.Create(XmlReader.Create(new StringReader(content))); | |
} | |
); | |
} |
if (childElement.ChildNodes.Count == 1 && childElement.FirstChild.NodeType == XmlNodeType.Text) | ||
{ | ||
// If the element has inner text and no other child elements except text, then this should be a property and throw invalid child element of <Target> | ||
ProjectXmlUtilities.ThrowProjectInvalidChildElement(childElement.Name, childElement.ParentNode.Name, childElement.Location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get a new, custom error message for this case, change this to something like
ProjectXmlUtilities.ThrowProjectInvalidChildElement(childElement.Name, childElement.ParentNode.Name, childElement.Location); | |
ProjectErrorUtilities.ThrowInvalidProject(childElement.Location, "PropertyOutsidePropertyGroupInTarget", childElement.Name); |
and add a new string near here:
msbuild/src/Build/Resources/Strings.resx
Lines 1270 to 1273 in d797c48
<data name="UnrecognizedChildElement" xml:space="preserve"> | |
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized.</value> | |
<comment>{StrBegin="MSB4067: "}</comment> | |
</data> |
src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to look great, just a few more things to wrap up!
src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs
Outdated
Show resolved
Hide resolved
src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me! Just a couple little things.
src/Build/Resources/Strings.resx
Outdated
@@ -1271,6 +1271,10 @@ | |||
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized.</value> | |||
<comment>{StrBegin="MSB4067: "}</comment> | |||
</data> | |||
<data name="PropertyOutsidePropertyGroupInTarget" xml:space="preserve" Condition="$([MSBuild]::AreFeaturesEnabled('17.6'))"> | |||
<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. Properties must be inside a <PropertyGroup> element.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. Properties must be inside a <PropertyGroup> element.</value> | |
<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value> |
? They might have been trying to do something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping on this point, which I think is the only outstanding issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add hint "If you intended this to be a property" for the new MSB4067?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please.
src/Build/Resources/Strings.resx
Outdated
@@ -1271,6 +1271,10 @@ | |||
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized.</value> | |||
<comment>{StrBegin="MSB4067: "}</comment> | |||
</data> | |||
<data name="PropertyOutsidePropertyGroupInTarget" xml:space="preserve" Condition="$([MSBuild]::AreFeaturesEnabled('17.6'))"> | |||
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value> | |
<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized. If you intended this to be a property, enclose it within a <PropertyGroup> element.</value> |
Otherwise you might interpret this to mean properties can't be inside targets, when they can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
collection.LoadProject(file.Path).Build().ShouldBeTrue(); | ||
}); | ||
|
||
var expectedString = "If you intended this to be a property, enclose it within a <PropertyGroup> element"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think I missed this before. Please don't check against English strings in unit tests, because that will make them fail on non-English OSes (after we get a translation for this specific string).
There aren't any tests in this assembly that do the check the most-correct way (extracting the resource by name).
Can you instead check for the code (MSB4067
) outside the if, and only if the new stuff is enabled additionally check that the message contains PropertyGroup
? That part of the string shouldn't be localized so I think that'd be a pretty good test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Update expected string in case the localization
Fixes #5773
Context
Put a property inside a target without an enclosing PropertyGroup and build. Error informing me that is an invalid child element of .
Changes Made
Add one condition when parse the element under target. If the element has inner text and no other child elements except text, then this should be a property and throw invalid child element of
Tests
ReadInvalidPropertyUnderTarget
Notes